Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

build: depend on bazelisk rather than directly on Bazel #36078

Closed
wants to merge 1 commit into from
Closed

build: depend on bazelisk rather than directly on Bazel #36078

wants to merge 1 commit into from

Conversation

gregmagolan
Copy link
Contributor

Based off of @alexeagle's PR #35153

This has a couple benefits:

  • we now use a .bazelversion file rather than package.json to pin the version of bazel we want. This means even if you install bazel on your computer rather than via yarn, you'll still get a warning if your bazel version is wrong.
  • you no longer end up downloading three copies of bazel due to bugs in both npm and yarn where they download all tarballs before checking the metadata to see which are usable on the local platform.
  • bazelisk correctly handles the tools/bazel trick for wrapping functionality, which we want to use to instrument developer build latencies

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@gregmagolan gregmagolan mentioned this pull request Mar 16, 2020
Closed
@gregmagolan gregmagolan force-pushed the bazelisk branch 3 times, most recently from e8289b9 to cc60fe5 Compare March 16, 2020 05:42
@alexeagle @gregmagolan
build: depend on bazelisk rather than directly on Bazel
c256f50 
This has a couple benefits:
- we now use a .bazelversion file rather than package.json to pin the version of bazel we want. This means even if you install bazel on your computer rather than via yarn, you'll still get a warning if your bazel version is wrong.
- you no longer end up downloading three copies of bazel due to bugs in both npm and yarn where they download all tarballs before checking the metadata to see which are usable on the local platform.
- bazelisk correctly handles the tools/bazel trick for wrapping functionality, which we want to use to instrument developer build latencies
@gregmagolan
Copy link
Contributor Author

Updating to Bazel 2.2.0 and bazel_toolchains 2.2.0 produces

ERROR: An error occurred during the fetch of repository 'rbe_ubuntu1604_angular':
   Traceback (most recent call last):
	File "/home/circleci/.cache/bazel/_bazel_circleci/9ce5c2144ecf75d11717c0aa41e45a8d/external/bazel_toolchains/rules/rbe_repo.bzl", line 505
		validate_host(ctx)
	File "/home/circleci/.cache/bazel/_bazel_circleci/9ce5c2144ecf75d11717c0aa41e45a8d/external/bazel_toolchains/rules/rbe_repo/util.bzl", line 161, in validate_host
		fail(<2 more arguments>)
attribute Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
: Cannot run rbe_autoconfig as running '%s ps' returned a non 0 exit code, please check you have permissions to run docker. Error message: /usr/bin/docker
ERROR: /home/circleci/ng/tools/BUILD.bazel:16:1: //tools:rbe_ubuntu1604-angular depends on @rbe_ubuntu1604_angular//config:platform in repository @rbe_ubuntu1604_angular which failed to fetch. no such package '@rbe_ubuntu1604_angular//config': Traceback (most recent call last):
	File "/home/circleci/.cache/bazel/_bazel_circleci/9ce5c2144ecf75d11717c0aa41e45a8d/external/bazel_toolchains/rules/rbe_repo.bzl", line 505
		validate_host(ctx)
	File "/home/circleci/.cache/bazel/_bazel_circleci/9ce5c2144ecf75d11717c0aa41e45a8d/external/bazel_toolchains/rules/rbe_repo/util.bzl", line 161, in validate_host
		fail(<2 more arguments>)
attribute Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
: Cannot run rbe_autoconfig as running '%s ps' returned a non 0 exit code, please check you have permissions to run docker. Error message: /usr/bin/docker
ERROR: While resolving toolchains for target //modules/e2e_util:e2e_util: Target //tools:rbe_ubuntu1604-angular was referenced as a platform, but does not provide PlatformInfo
ERROR: Analysis of target '//modules/e2e_util:e2e_util_es5' failed; build aborted: Analysis failed

Which I've seen before when I first tried out 2.0.0 but it was fixed in a subsequent toolchain patch version.
Pinning bazel to 2.1.1 for now.

@gregmagolan gregmagolan added target: major This PR is targeted for the next major release area: build & ci Related the build and CI infrastructure of the project labels Mar 16, 2020
@ngbot ngbot bot added this to the needsTriage milestone Mar 16, 2020
@gregmagolan gregmagolan marked this pull request as ready for review March 16, 2020 06:17
@pullapprove pullapprove bot requested a review from IgorMinar March 16, 2020 06:17
josephperrott
josephperrott reviewed Mar 16, 2020
View reviewed changes
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for picking this up!

@gregmagolan gregmagolan removed the request for review from IgorMinar March 16, 2020 15:13
@gregmagolan gregmagolan added the action: merge The PR is ready for merge by the caretaker label Mar 16, 2020
@AndrewKushnir
Copy link
Contributor

Hi @gregmagolan, would it make sense to run Windows CI jobs for these changes as well (like you did for other PRs)? Thank you.

@gregmagolan
Copy link
Contributor Author

Yes it would. I'll make a PR.

gregmagolan added a commit to gregmagolan/angular that referenced this pull request Mar 16, 2020
@gregmagolan
build: windows ci for angular#36078 [DO NOT MERGE]
2edf79a
@gregmagolan gregmagolan mentioned this pull request Mar 16, 2020
Closed
@gregmagolan
Copy link
Contributor Author

Windows CI running here: #36088

@AndrewKushnir
Copy link
Contributor

Thanks @gregmagolan! 👍 I'll put the "blocked" label on this PR for now, feel free to remove it once Windows CI is completed successfully. Thank you.

@gregmagolan
Copy link
Contributor Author

Thanks @AndrewKushnir . Windows CI green #36088.

@AndrewKushnir
Copy link
Contributor

Thanks @gregmagolan 👍 I've just merged this PR into master (according to the PR target label). Do we need this change in patch branch as well or no?

@gregmagolan
Copy link
Contributor Author

No. Patch branch is behind on Bazel changes due to an incompat with ts_library so only master until the next patch branch.

josephperrott added a commit to josephperrott/angular that referenced this pull request Mar 17, 2020
@josephperrott
fixup! build: depend on bazelisk rather than directly on Bazel (angul…
257ac42 
…ar#36078)
@devversion devversion mentioned this pull request Mar 18, 2020
Closed
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@josephperrott josephperrott josephperrott approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@gregmagolan @AndrewKushnir @josephperrott @googlebot @alexeagle